Add Detekt docs and reorganise existing readme/docs #54

rock3r · 2018-01-30T19:00:03Z

This PR:

Fixes the Apache 2.0 license setup, adding the NOTICE file and reformatting/fixing LICENSE (was LICENSE.txt)
Improves the readme file, trimming it down to the bare essentials to get people going without overwhelming them
Moves the advanced configuration docs to the docs folder
Adds detailed informations about all supported tools, in the docs folder
Many more small tweaks and grammar improvements

tasomaniac · 2018-01-30T20:25:01Z

docs/supported-tools.md

+with an error like:
+
+```
+The detekt plugin is configured but not applied. Please apply the plugin in your build script.


Do we really need to go into this much details? Above paragraph is I think really good but the duplication of the error message is redundant. I don't think this will be helpful to anybody.

Reasoning: you get the error message and search for it. You get a hit, with the resolution steps. Everyone's happy.

Then I would put a link to our docs in the error message. Rather than having it duplicated here.
They would see the error and click the link, that's all. Don't have to search for something.

Duplication is bad because the error may change and it is hard to maintain this copy to be the same.

That's a good idea, tomorrow I will change #53 for that to change the text here and add a link to this bit.

I will postpone this to after I have split out the tools docs, one per file, so I don't have to go back and change things again. I need #57 merged, and then this one merged, to be able to proceed with this.

That sounds good.

tasomaniac · 2018-01-30T20:26:40Z

docs/supported-tools.md

+
+In order to use Detekt in your project, you need to:
+
+ 1. Add this statement to your root `build.gradle` project (change the version according to your needs):


Actually this is not entirely true. In a simpler project where there is only 1 subproject, you can directly have this there without having anything in root at all.

Well, not true, because Detekt expects to be applied to the root project, and then to all relevant subprojects.

Either works. You don't have to have it in the root. If you have it in the root, then you don't need to duplicate the version.

My goal around this comment actually was that I think we have too much information here.

If you have it in the root, then you don't need to duplicate the version.

In the case you were talking about initially, there is only the root, so there is no duplication. In the "normal" case that this guide covers, there is no duplication either since the version is only written in one place, the root build.gradle.

My goal around this comment actually was that I think we have too much information here.

Do you mean it's bad having the minimum instructions to make the project work? I mean, this is literally the minimum amount of steps required for the Detekt integration to work. If you meant that, as things are, adding Detekt support through SAP is a painful process that requires a lot of explanations to a new starters, I absolutely agree — I maintain that not applying the plugin ourselves makes this UX rather unsatisfactory and it is not fine to leave things as they are in the long term

Let me clarify :) What I mean is that our plugin does not care how Detekt plugin is added. We just expect it to be available.
There are many ways to add a plugin. And can also change in the future. So that's why I thought we can leave it to the official docs. But it is not really that important.

Ok, a bit clearer now :) Well, if your concern is just that we should not mandate how the Detekt plugin is added, we may just want to rephrase the text before the list from:

In order to use Detekt in your project, you need to:

to:

In order to use Detekt, you need to manually add it to **all** your Kotlin projects. You can refer to the [official documentation](https://github.com/arturbosch/detekt/#gradlegroovy) for further details. Note that you should _not_ add the `detekt` closure to your `build.gradle`s, unlike what the official documentation says. The `detekt` closure in the `staticAnalysis` configuration gets applied to all Kotlin modules automatically. In most common cases, adding Detekt to a project boils down to three simple steps: [...]

It is more verbose (mostly because of the not adding detekt stuff) than just having what we have now, but if you refer to the official docs you must point out not to add the detekt configuration — lest things not work as expected. That was the reason why, as I explained earlier, I didn't refer users to the official docs (which by the way make a very poor job at explaining what to do).

More verbose but at the same time more maintainable.
I personally like the last one you suggested in above comment.

tasomaniac · 2018-01-30T20:28:08Z

docs/supported-tools.md

+For more information see https://github.com/arturbosch/detekt.
+```
+
+In order to use Detekt in your project, you need to:


What about having a link to official Detekt documentation? Users will definitely have to go there for details of the configuration anyways. So we would like them to go there and use their docs to learn how to add. And then use our docs to setup.

There is several already, specifically pointing to the relevant bits, where applicable. Our instructions on how to add are different enough from the official ones that they are conflicting, so I chose not to add a direct reference here (but there are in the next sections)

My goal was to keep this docs brief. Considering we will have more tools, having too much information may hurt.
"How to add detekt" is out of our concern, imho. We expect it to be added, that's all.

I think it would be a grave omission not to have this information here — after all you suggest yourself at line 222 to link to this documentation in the error message.

When we'll add more tools, we'll begin splitting this out into separate docs. I'm already planning that, but it doesn't really make much sense right now imho. I will do it when we add Lint.

After the comment from @mr-archano below asking for a split I put some more thought into the split. I will do it, before we get to Lint, but not in this PR because we'd lose the majority of the comments and discussions. A new PR will follow today with that change.

tasomaniac · 2018-01-30T20:31:31Z

README.md

+- convenient way of sharing same setup across different projects
+- healthy, versionable and configurable defaults
+
+### Supported static analysis tools


Supported tools to have a shorter title?

Same for me, doesn't really change much ¯\_(ツ)_/¯

tasomaniac · 2018-01-30T20:32:10Z

README.md

+Please note that the tools availability depends on the project the plugin is applied to. For more details please refer to the
+[supported tools](docs/supported-tools.md) page.
+
+### Out-of-the-box support for Android and Kotlin projects


Title includes kotlin but the content does not mention anything kotlin related.

Not much to mention besides that? I was not sure what to add. Suggestions welcome.

If we don't, maybe we can remove Kotlin from this header? Having support for detekt ktlint already indicates that we support those tools.

Well, does it not imply it just as much as mentioning Android Lint implies support for Android projects? I don't really see the problem here, to be honest.

I agree with @tasomaniac, imho we could remove Kotlin from the headline. Additionally we could be a bit more explicit by mentioning PMD, Findbugs and Checkstyle as problematic when it comes to Android Projects.

We have a special paragraph to talk about Android because the default tools needs setup for Android and does not work out of the box.
That's why this paragraph is really good.
We don't have anything special around Kotlin, imo

tasomaniac · 2018-01-30T20:33:38Z

README.md

@@ -1,23 +1,43 @@
-# gradle-static-analysis-plugin
-[![](https://ci.novoda.com/buildStatus/icon?job=gradle-static-analysis-plugin)](https://ci.novoda.com/job/gradle-static-analysis-plugin/lastSuccessfulBuild/console) [![](https://img.shields.io/badge/License-Apache%202.0-lightgrey.svg)](LICENSE.txt) [![Bintray](https://api.bintray.com/packages/novoda/maven/gradle-static-analysis-plugin/images/download.svg) ](https://bintray.com/novoda/maven/gradle-static-analysis-plugin/_latestVersion)
+# Gradle static analysis plugin


What about having a table of contents like section to have links to advanced configuration etc? Because otherwise, I'm afraid people will not see the inlined links.

That wouldn't be a TOC of this document... besides there's extremely little content right now, I don't think it's easy to miss the links

mr-archano

LGTM overall, it's quite some improvement from what we had. I noticed some things were still missing in the readme, I will try to extend this documentation once this PR is merged.

mr-archano · 2018-01-30T21:48:54Z

docs/supported-tools.md

+}
+```
+
+## Checkstyle


@rock3r what about creating a separate md file for each tool?

As mentioned above, was planning to do it when adding Lint. Can do in this PR if consensus is going that way.

Continues in #54 (comment)

tobiasheine

good job 👍
just some minor remarks

tobiasheine · 2018-01-31T08:25:15Z

README.md

+Please note that the tools availability depends on the project the plugin is applied to. For more details please refer to the
+[supported tools](docs/supported-tools.md) page.
+
+### Out-of-the-box support for Android and Kotlin projects


I agree with @tasomaniac, imho we could remove Kotlin from the headline. Additionally we could be a bit more explicit by mentioning PMD, Findbugs and Checkstyle as problematic when it comes to Android Projects.

tobiasheine · 2018-01-31T08:28:47Z

docs/advanced-usage.md

+Groovydocs.
+
+## Add exclusions with `exclude` filters
+You can specify custom patterns to exclude specific files from the static analysis. All you have to do is to specify `exclude`


This doesn't work for Detekt as of now. Should we mention that?

Yes. Good spot! I hadn't looked at the Detekt config yet when I checked this :)

tobiasheine · 2018-01-31T08:29:07Z

docs/advanced-usage.md

+```
+
+## Add exclusions with Android build variants
+Sometimes using `exclude` filters could be not enough. When using the plugin in an Android project you may want to consider


Same as above

tobiasheine · 2018-01-31T08:32:53Z

docs/supported-tools.md

+ }
+ ```
+
+### Configure Detekt


Should we add that we suggest to disable thresholds in the detekt config file?

Good point. Since we cannot automatically do this, it is really important to mention.

Yes! I totally forgot about it!

…_readme

Tweak Detekt exception message

rock3r · 2018-01-31T11:32:50Z

I have added some explanation on how to exclude files from Detekt. Unfortunately the official docs do not really explain that, so we have to do it ourselves.

rock3r · 2018-01-31T11:35:26Z

@tasomaniac @mr-archano @tobiasheine all comments addressed. Will start working on splitting out the docs for each tool in their own file next.

tobiasheine · 2018-01-31T11:40:42Z

@rock3r Do you wanna do the split as part of this PR or can we merge it?

tasomaniac · 2018-01-31T11:42:42Z

Let's just merge it. 👍

rock3r added 14 commits January 29, 2018 17:14

Improve readme file

3e91310

Fix Apache 2.0 licence

d453d9c

Move non-essential content out of the readme

21d3533

Clarify supported tools table in README

8cf0a9e

Reorganise advanced-usage contents

dfc590a

Add TOC to advanced-usage

8930952

Create supported-tools

691c20d

Simplify README, point to pages in docs/

accf4fc

Add missing whiteline at EOF in NOTICE

7c43d55

Fix Android Lint support matrix

d4c6ef0

Add documentation for the Checkstyle plugin

ce97e86

Add instructions for enabling/disabling tools

1b32738

Use defaults in config in the README file

e0e5ff4

Add documentation for all tools to supported-tools

0577d74

rock3r requested review from tobiasheine and mr-archano January 30, 2018 19:00

tasomaniac reviewed Jan 30, 2018

View reviewed changes

mr-archano approved these changes Jan 30, 2018

View reviewed changes

tobiasheine reviewed Jan 31, 2018

View reviewed changes

rock3r and others added 10 commits January 31, 2018 09:28

Merge branch 'PT-491/integrate_detekt' into add_detekt_and_reorganise…

f7074d1

…_readme

Fix grammar in error message

fb10e7f

Cleanup DetektConfigurator

f0d2803

Cleanup LogsSubject

2a02420

Merge pull request #57 from novoda/tweak-detekt-exception-message

267b4bf

Tweak Detekt exception message

Shorten a title in README

f9b8f07

Remove "Kotlin" from a title in README

cb7a7d4

Note that excludes don't work for Detekt

21430dc

Note not to configure limits or failFast for Detekt

a38c70b

Explain how to exclude files from Detekt analysis

b6e2e1e

Tweak wording for adding Detekt to a project

fadaac3

tobiasheine approved these changes Jan 31, 2018

View reviewed changes

tasomaniac merged commit 0458a77 into PT-491/integrate_detekt Jan 31, 2018

tasomaniac deleted the add_detekt_and_reorganise_readme branch January 31, 2018 11:42

tobiasheine mentioned this pull request Feb 1, 2018

[PT-491]: Integrate Detekt #58

Merged


		In order to use Detekt in your project, you need to:

		1. Add this statement to your root `build.gradle` project (change the version according to your needs):

Add Detekt docs and reorganise existing readme/docs #54

Add Detekt docs and reorganise existing readme/docs #54

Conversation

rock3r commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rock3r Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rock3r Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rock3r Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr-archano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasheine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rock3r Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rock3r commented Jan 31, 2018

rock3r commented Jan 31, 2018

tobiasheine commented Jan 31, 2018

tasomaniac commented Jan 31, 2018

rock3r Jan 30, 2018 •

edited

Loading

rock3r Jan 30, 2018 •

edited

Loading

rock3r Jan 30, 2018 •

edited

Loading

rock3r Jan 31, 2018 •

edited

Loading